-
Notifications
You must be signed in to change notification settings - Fork 149
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Include directory entries when building wheel #289
Conversation
Codecov Report
@@ Coverage Diff @@
## master #289 +/- ##
==========================================
+ Coverage 59.59% 60.07% +0.47%
==========================================
Files 12 12
Lines 839 849 +10
==========================================
+ Hits 500 510 +10
Misses 339 339
Continue to review full report at Codecov.
|
def writestr(self, zinfo_or_arcname, bytes, compress_type=None): | ||
ZipFile.writestr(self, zinfo_or_arcname, bytes, compress_type) | ||
fname = (zinfo_or_arcname.filename if isinstance(zinfo_or_arcname, ZipInfo) | ||
else zinfo_or_arcname) | ||
logger.info("adding '%s'", fname) | ||
if fname != self.record_path: | ||
if fname != self.record_path and not fname.endswith('/'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this check really necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you take it out, the tests fail because there are RECORD entries for the directories (with a SHA hash for an empty file).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I see it's also used by the mkdir()
method.
build_dir = tmpdir | ||
sub_dir = build_dir / 'sub' | ||
sub_dir.mkdir() | ||
(sub_dir / '__init__.py').write_text('', encoding='utf-8') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why create a file in the directory? Wasn't the point to ensure that empty directories get included, or did I misunderstand?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. I decided that it's probably better to match the behavior observed in other tools that generate zip files from a tree and create directory entries for every directory. But I do take from your point that the test should probably also ensure that directory entries are created for empty directories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 0e94b10
Woo! Thanks. |
See #287 for background.